Skip to content

[discuss] Enables to resolve direct pom dependencies (not transitive ones) location as project.dependencies. reference - eases javaagent usage for ex - #1281

Closed
rmannibucau wants to merge 2 commits into
apache:masterfrom
rmannibucau:fb/direct-dependencies-as-properties
Closed

Conversation

@rmannibucau

Copy link
Copy Markdown
Contributor

Sample proposal to try to ease mockito/mockito#3037.

Main issue of this PR is to not support transitive deps but think it is not a bad compromise (using javaagent "scope" would be as bad + totally wrong/broken in terms of design - would loose the scope to be clear, and adding an attribute sounds overkill until we get something like gradle configuration).
Main advantage is it makes it easy to work for everybody and stays aligned on the JVM (explicit toggling with ordering) for javaagent cases.

So overall think it can be a good compromise. If validated we have to document it properly - probably in maven site and surefire one?

@bmarwell if you want to try/take this over, but thought a PR would be easier than slack ;)

…tion as project.dependencies. reference - eases javaagent usage for ex
@bmarwell

Copy link
Copy Markdown
Contributor

So the idea is that a given group and artifact ID gets resolved to the following property:

${project.dependencies.com.bar.dummy-agent}

Only disadvantage of your solution: I don't see where the groupId ends and the artifact ID begins. But that's probably not going to clash.

This is very helpful for agents.

Other ideas:

Change the default lifecycle XML to include the property goal of the dependency plugin. Way smaller change.

@bmarwell

Copy link
Copy Markdown
Contributor

I forgot this file doesn't exist anymore.

https://github.com/apache/maven/blob/maven-3.3.8/maven-core/src/main/resources/META-INF/plexus/default-bindings.xml

But I think you get what I'm referring to.

@rmannibucau

Copy link
Copy Markdown
Contributor Author

If it clashes we have other troubles and using another sep looks wrong in placeholders to me.
Im also very hesitant to add another plugin in any lifecycle cause:

  1. It is specific to default lifecycles
  2. Adds another prerequisite and i tend to think we already have too much
  3. It will be too late in the interpolation and need other passes

@bmarwell

Copy link
Copy Markdown
Contributor

Thank you for the explanation, that makes excellent documentation! 😃

@gnodet

gnodet commented Oct 18, 2023

Copy link
Copy Markdown
Contributor

This really looks like a temporary hack for agents to being supported by Surefire...
It only saves the following declaration:

 <plugin>
    <artifactId>maven-dependency-plugin</artifactId>
    <executions>
      <execution>
        <goals>
          <goal>properties</goal>
        </goals>
      </execution>
    </executions>
  </plugin>

@gnodet gnodet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now read the full mockito thread. I get a better understand. While I'd be ok with the goal provided by the PR (ease or configuration for agents ), I think the current PR is wrong as it is implemented when the model is built, before dependencies are resolved. That means the version will be empty in most cases (when using managed dependencies).
So I'm not sure the current PR is worth it.

@rmannibucau

Copy link
Copy Markdown
Contributor Author

Well any later phase is wrong cause then it depends if the interpolation will be reprocessed or not so think it is the least bad compromise to work immediately but agree on your point, the limitation is to explicit define the dependency in the project (potentially using a property but without anything implicit).
I'm fine dropping this PR if it does not help while we don't add dependency plugin in any default lifecycle.

@gnodet

gnodet commented Oct 18, 2023

Copy link
Copy Markdown
Contributor

Well any later phase is wrong cause then it depends if the interpolation will be reprocessed or not so think it is the least bad compromise to work immediately but agree on your point, the limitation is to explicit define the dependency in the project (potentially using a property but without anything implicit). I'm fine dropping this PR if it does not help while we don't add dependency plugin in any default lifecycle.

@rmannibucau Wouldn't it be better to enhance the PluginParameterExpressionEvaluatorV4 class instead ?

@rmannibucau

Copy link
Copy Markdown
Contributor Author

@gnodet I'm mixed about it since it assumes everything is wired in plugins conf and plugins/extensions don't read any other property so not sure it is really better today, maybe let's wait to see what mockito picks?

@gnodet
gnodet marked this pull request as draft October 19, 2023 07:45
@rmannibucau

Copy link
Copy Markdown
Contributor Author

solved in maven 4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants